Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating dio_read_verify ZTS test #16830

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bwatkinson
Copy link
Contributor

There was a recent CI ZTS test failure on FreeBSD 14 for the dio_read_verify test case. The failure reported there was no ARC reads while the buffer wes being manipulated. All checksum verify errors for Direct I/O reads are rerouted through the ARC, so there should be ARC reads accounted for. In order to help debug any future failures of this test case, the order of checks has been changed. First there is a check for DIO verify failures for the reads and then ARC read counts are checked.

This PR also contains general cleanup of the comments in the test script.

Reordering some calls in dio_read_verify test to help with debugging recent CI test failure in FreeBSD 14.

Motivation and Context

This change will allow for further debugging in the event of another dio_read_verify test failure in the CI runs. It is helpful first to see if there are DIO read verify failures before checking if there are any ARC reads.

Description

Reordered checks in dio_read_verify. First there are checks to make sure there are in fact DIO read verify failures before checking there are ARC reads. There always should be DIO read verify failures, as the buffer is constantly being manipulated while Direct I/O reads are taking place in the test.

How Has This Been Tested?

Tested locally on Linux kernel 4.18.0-408.el8.x86_64.

Ran direct ZTS tests for multiple iterations without failure after updating this test case.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@bwatkinson bwatkinson requested a review from amotin December 2, 2024 23:36
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 3, 2024
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder would it be more logical to first check for Direct I/O reads before Direct I/O errors? Otherwise have no objections.

@bwatkinson bwatkinson force-pushed the dio_read_verify_cleanup branch from 2b5d806 to e85ba3d Compare December 3, 2024 23:21
@bwatkinson
Copy link
Contributor Author

I wonder would it be more logical to first check for Direct I/O reads before Direct I/O errors? Otherwise have no objections.

Fine by me. I just updated the ordering of the checks in the test case.

@bwatkinson bwatkinson requested a review from amotin December 3, 2024 23:22
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant such check order, matching the order of events: Direct I/O reads, Direct I/O checksum errors, ARC reads, no ARC errors. Is my logic wrong? But whatever...

There was a recent CI ZTS test failure on FreeBSD 14 for the
dio_read_verify test case. The failure reported there was no ARC reads
while the buffer wes being manipulated. All checksum verify errors for
Direct I/O reads are rerouted through the ARC, so there should be ARC
reads accounted for. In order to help debug any future failures of this
test case, the order of checks has been changed. First there is a check
for DIO verify failures for the reads and then ARC read counts are
checked.

This PR also contains general cleanup of the comments in the test
script.

Signed-off-by: Brian Atkinson <[email protected]>
@bwatkinson bwatkinson force-pushed the dio_read_verify_cleanup branch from e85ba3d to ee0c601 Compare December 4, 2024 18:17
@bwatkinson
Copy link
Contributor Author

I meant such check order, matching the order of events: Direct I/O reads, Direct I/O checksum errors, ARC reads, no ARC errors. Is my logic wrong? But whatever...

Fine by me. I updated the ordering again.

@amotin amotin added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 4, 2024
@behlendorf behlendorf added Status: Revision Needed Changes are required for the PR to be accepted and removed Status: Accepted Ready to integrate (reviewed, tested) labels Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Revision Needed Changes are required for the PR to be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants